Script Loading Strategy - ML2 #13 - Add `get_eligible_loading_strategy` method #35

kt-12 · 2023-02-23T21:01:56Z

Trac ticket:

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

…get_eligible_loading_strategy-ML2-12

…enhancement/do_item-ML2-13

…enhancement/do_item-ML2-14

joemcgill

Leaving feedback (mostly formatting related).

As a process note, I'm find it a bit difficult to follow the way you have PRs set up to merge into milestone branches before merging to the main feature branch. Given that this is meant to be a continuation from #34, I'd love to see us get that merged into the base feature branch you've set up and then set each of these M2 branches to merge into the main feature branch, rather than waiting for all of it to land "at once" as part of a milestone. That way, we're also getting the benefit of seeing the test coverage increase over time, with each PR and will more likely avoid merge conflicts at the end.

src/wp-includes/class-wp-scripts.php

joemcgill · 2023-02-24T21:07:57Z

src/wp-includes/class-wp-scripts.php

+		}
+
+		// Handling async strategy scenarios.
+		if ( empty( $this->registered[ $handle ]->deps ) && empty( $this->get_dependents( $handle ) ) ) {


I'd love to see some more test cases set up to confirm that each scenario you're accounting for in this logic tree is working as expected.

@joemcgill Test cases are the main reason I had to set a base milestone branch. The way our milestone tasks are, we normally have to get to the last task to test out all the other prior tasks in the milestone.

In milestone 2, get_eligible_strategy can only be tested via do_item method ( #38 ) which is a part of different task in same milestone. In milestone 2 all test cases are in PR #38.

Just to be certain that everything in a single milestone works as intended before it goes to the base branch, i created a separate milestone base branch.

…enhancement/do_item-ML2-14

src/wp-includes/class-wp-scripts.php

10upsimon · 2023-02-27T16:39:34Z

src/wp-includes/class-wp-scripts.php

+			return 'async';
+		}
+
+		// Handling defer strategy scenarios. Dependency will never be set async. So only checking dependent.


@kt-12 Do we need to mention Dependency will never be set async. So only checking dependent. here? I am not following what this means, did you mean to state that dependencies will never be deferred so you're only checking dependents?

@10upsimon So, per the rules, defer can only have 'defer' or 'blocking' dependencies; it can never be 'async'. ( point 1 )

A decision that a script is to be set to async, defer, or nothing is made by get_eligible_loading_strategy.

But, if you check the rules for async in this function, a script with dependent will never be set async. ( point 2 ).

From points 1 and 2, I can say if the current script has some dependency, the dependency script will never have async strategy (as it has a dependent). So we can avoid a recursion check to find strategy in dependencies.

I hope I still don't sound confusing.

@10upsimon I have removed the extra text here.

10upsimon · 2023-02-27T16:42:27Z

For visibility sake, as I myself wondered where the unit tests were that handled the various loading strategies, they've been added to the PR for #14 and can be seen at https://github.com/10up/wordpress-develop/pull/38/files#diff-09a2e7959959d6a08ef07b1849e0178b853f7ef86f7885bbf9be2ed261145b24

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

…enhancement/do_item-ML2-14

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

joemcgill

The updates are looking good. I left a couple new questions and will do a final review once we have the tests from #14 merged back here so we can ensure everything is covered.

joemcgill · 2023-02-27T21:08:50Z

src/wp-includes/class-wp-scripts.php

+	 * Get the strategy mentioned during script registration.
+	 *
+	 * @param string $handle The script handle.
+	 * @return string|bool Strategy in script registration, False if not strategy is mentioned..


Suggested change

* @return string|bool Strategy in script registration, False if not strategy is mentioned..

* @return string|bool Strategy in script registration, False if not strategy is mentioned.

src/wp-includes/class-wp-scripts.php

tests/phpunit/tests/dependencies/scripts.php

joemcgill · 2023-02-27T21:23:34Z

src/wp-includes/class-wp-scripts.php

+		 * blocking if explicitly set.
+		 * blocking if script args not set.
+		 */
+		if ( ! isset( $this->registered[ $handle ] ) || 'blocking' === $intended_strategy || ! $intended_strategy ) {


I'm still confused about why script registration is being checked here after the intended strategy instead of before? Can you explain the use case for me?

I'd also suggest splitting this back to multiple lines for readability, just make sure the operators are at the beginning of each line, like the previous suggestion.

You are correct. script registration should be above getting intended script. I have made the change.

Now that the condition have become much smaller I have kept it on the same line.

10upsimon · 2023-02-28T12:01:05Z

@joemcgill in case you missed it, noting that the PR at #34 was merged into the same base branch this is going to 👍

Co-authored-by: Joe McGill <801097+joemcgill@users.noreply.github.com>

…enhancement/do_item-ML2-14

src/wp-includes/class-wp-scripts.php

kt-12 · 2023-02-24T02:01:24Z

src/wp-includes/class-wp-scripts.php

+	 * @param string $handle The script handle.
+	 * @return array Array of script handles.
+	 */
+	private function get_dependents( $handle ) {


Note: This needs to be optimised further. Better way of doing this is creating a dependents list as a hash table (added as a Dependencies to WP_Script) and updated dependents while each script is getting registered. get_dependent will simply fetch dependent from that hash table.

src/wp-includes/class-wp-scripts.php

kt-12 · 2023-02-26T05:12:26Z

src/wp-includes/class-wp-scripts.php

+		}
+
+		// Handling async strategy scenarios.
+		if ( empty( $this->registered[ $handle ]->deps ) && empty( $this->get_dependents( $handle ) ) ) {


@joemcgill Test cases are the main reason I had to set a base milestone branch. The way our milestone tasks are, we normally have to get to the last task to test out all the other prior tasks in the milestone.

In milestone 2, get_eligible_strategy can only be tested via do_item method ( #38 ) which is a part of different task in same milestone. In milestone 2 all test cases are in PR #38.

Just to be certain that everything in a single milestone works as intended before it goes to the base branch, i created a separate milestone base branch.

kt-12 · 2023-02-27T18:30:47Z

src/wp-includes/class-wp-scripts.php

+			return 'async';
+		}
+
+		// Handling defer strategy scenarios. Dependency will never be set async. So only checking dependent.


@10upsimon So, per the rules, defer can only have 'defer' or 'blocking' dependencies; it can never be 'async'. ( point 1 )

A decision that a script is to be set to async, defer, or nothing is made by get_eligible_loading_strategy.

But, if you check the rules for async in this function, a script with dependent will never be set async. ( point 2 ).

From points 1 and 2, I can say if the current script has some dependency, the dependency script will never have async strategy (as it has a dependent). So we can avoid a recursion check to find strategy in dependencies.

I hope I still don't sound confusing.

src/wp-includes/class-wp-scripts.php

kt-12 · 2023-02-28T15:38:35Z

src/wp-includes/class-wp-scripts.php

+		 * blocking if explicitly set.
+		 * blocking if script args not set.
+		 */
+		if ( ! isset( $this->registered[ $handle ] ) || 'blocking' === $intended_strategy || ! $intended_strategy ) {


You are correct. script registration should be above getting intended script. I have made the change.

Now that the condition have become much smaller I have kept it on the same line.

kt-12 · 2023-02-28T15:39:24Z

src/wp-includes/class-wp-scripts.php

+			return 'async';
+		}
+
+		// Handling defer strategy scenarios. Dependency will never be set async. So only checking dependent.


@10upsimon I have removed the extra text here.

tests/phpunit/tests/dependencies/scripts.php

src/wp-includes/class-wp-scripts.php

joemcgill

I think this is looking good now 👍🏻

Script Loading Strategy - ML2 #14 - Update `do_item` method.

kt-12 added 4 commits February 22, 2023 13:54

get_eligible_loading_strategy logic

20917f9

Deferring strategy refined

db02a89

Merge branch 'enhancement/wp-script-api-milestone1' into enhancement/…

1114f35

…get_eligible_loading_strategy-ML2-12

Bug fix

af30f17

kt-12 added this to the Milestone 2 milestone Feb 23, 2023

kt-12 linked an issue Feb 23, 2023 that may be closed by this pull request

Addition of get_eligible_loading_strategy() method to surface correct strategy for a given script handle #13

Closed

1 task

kt-12 added 3 commits February 24, 2023 01:42

Merge branch 'enhancement/wp-script-api-milestone2' into enhancement/…

1d3c887

…get_eligible_loading_strategy-ML2-12

removing a few lines with function

2ed92d3

removing default script strategy

552d91a

kt-12 changed the title ~~Script Loading Strategy - ML2 issue 13~~ Script Loading Strategy - ML2 issue 13 add get_eligible_loading_strategy method Feb 24, 2023

do_item strategy support

1eacd05

kt-12 changed the base branch from enhancement/wp-script-api-milestone1 to enhancement/wp-script-api-milestone2 February 24, 2023 09:50

kt-12 added 2 commits February 24, 2023 09:56

bug fix

4542775

Merge branch 'enhancement/get_eligible_loading_strategy-ML2-12' into …

60b460a

…enhancement/do_item-ML2-13

kt-12 changed the title ~~Script Loading Strategy - ML2 issue 13 add get_eligible_loading_strategy method~~ Script Loading Strategy - ML2 #13 - Add get_eligible_loading_strategy method Feb 24, 2023

10upsimon mentioned this pull request Feb 24, 2023

Documentation - Inline (Automated) & Community #29

Closed

1 task

kt-12 mentioned this pull request Feb 24, 2023

Addition of get_eligible_loading_strategy() method to surface correct strategy for a given script handle #13

Closed

1 task

kt-12 added 3 commits February 24, 2023 19:05

Strategy name update.

d89ab9a

fixing bug

4105504

Merge branch 'enhancement/get_eligible_loading_strategy-ML2-12' into …

ceb670f

…enhancement/do_item-ML2-14

joemcgill reviewed Feb 24, 2023

View reviewed changes

Base automatically changed from enhancement/wp-script-api-milestone2 to enhancement/wp-script-api-strategy-base February 25, 2023 01:46

kt-12 added 8 commits February 26, 2023 04:13

strategy test cases

a9eef9f

cs fixes

c2c1d07

opt-in + remove blocking after dep

9eb3f89

Merge branch 'enhancement/get_eligible_loading_strategy-ML2-12' into …

6cc7dec

…enhancement/do_item-ML2-14

complex condition towards end

35279fc

Merge branch 'enhancement/get_eligible_loading_strategy-ML2-12' into …

74e4aef

…enhancement/do_item-ML2-14

if user not entered

1de0f31

place comment in order

d5ebbc3

kt-12 added the needs:code-review This requires code review. label Feb 27, 2023

kt-12 added 2 commits February 27, 2023 09:57

remove after dependency function

fcaa62e

Merge branch 'enhancement/get_eligible_loading_strategy-ML2-12' into …

150ba8b

…enhancement/do_item-ML2-14

10upsimon requested changes Feb 27, 2023

View reviewed changes

kt-12 and others added 7 commits February 27, 2023 18:31

fix doc block text

d578e50

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

Correct grammar

5262bf7

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

Fix doc block string.

2970f1a

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

change variable name

28af60d

doc block changes, better word

f746dfd

Merge branch 'enhancement/get_eligible_loading_strategy-ML2-12' into …

00a50d1

…enhancement/do_item-ML2-14

spacing issue.

d4bbaff

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

joemcgill reviewed Feb 27, 2023

View reviewed changes

kt-12 and others added 8 commits February 28, 2023 12:55

update placeholder with numbered placeholder

01e1d4f

Co-authored-by: Joe McGill <801097+joemcgill@users.noreply.github.com>

Change ticket id to trac id

a367951

Switching from numbered to normal substitution

51e3e41

Breaking test into multiple test functions

62cbf60

breaking test to dataprovider

0f80794

add async and defer check in dependent check

f20cfea

Merge branch 'enhancement/get_eligible_loading_strategy-ML2-12' into …

e406a13

…enhancement/do_item-ML2-14

defer with async dependent

b961108

kt-12 commented Feb 28, 2023

View reviewed changes

joemcgill approved these changes Feb 28, 2023

View reviewed changes

kt-12 and others added 2 commits February 28, 2023 18:39

Add failure messages

70fb6c6

Merge pull request #38 from 10up/enhancement/do_item-ML2-14

2b3d11c

Script Loading Strategy - ML2 #14 - Update `do_item` method.

kt-12 requested a review from 10upsimon February 28, 2023 18:41

10upsimon approved these changes Mar 1, 2023

View reviewed changes

kt-12 merged commit 16c4634 into enhancement/wp-script-api-strategy-base Mar 1, 2023

kt-12 deleted the enhancement/get_eligible_loading_strategy-ML2-12 branch March 1, 2023 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Script Loading Strategy - ML2 #13 - Add `get_eligible_loading_strategy` method #35

Script Loading Strategy - ML2 #13 - Add `get_eligible_loading_strategy` method #35

kt-12 commented Feb 23, 2023

joemcgill left a comment

joemcgill Feb 24, 2023

kt-12 Feb 26, 2023

10upsimon Feb 27, 2023

kt-12 Feb 27, 2023

kt-12 Feb 28, 2023

10upsimon commented Feb 27, 2023

joemcgill left a comment

joemcgill Feb 27, 2023

joemcgill Feb 27, 2023

kt-12 Feb 28, 2023

10upsimon commented Feb 28, 2023

kt-12 Feb 24, 2023

kt-12 Feb 26, 2023

kt-12 Feb 27, 2023

kt-12 Feb 28, 2023

kt-12 Feb 28, 2023

joemcgill left a comment

	* @return string\|bool Strategy in script registration, False if not strategy is mentioned..
	* @return string\|bool Strategy in script registration, False if not strategy is mentioned.

Script Loading Strategy - ML2 #13 - Add get_eligible_loading_strategy method #35

Script Loading Strategy - ML2 #13 - Add get_eligible_loading_strategy method #35

Conversation

kt-12 commented Feb 23, 2023

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

10upsimon commented Feb 27, 2023

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

10upsimon commented Feb 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

Script Loading Strategy - ML2 #13 - Add `get_eligible_loading_strategy` method #35

Script Loading Strategy - ML2 #13 - Add `get_eligible_loading_strategy` method #35